Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cache swift authentication token in memcache #4116

Merged
merged 3 commits into from
Mar 29, 2017
Merged

Cache swift authentication token in memcache #4116

merged 3 commits into from
Mar 29, 2017

Conversation

icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Mar 28, 2017 •
edited by MorrisJobke
Loading

Prevents having to do an authentication roundtrip every request.

Requires

@mention-bot
Copy link

@icewind1991, thanks for your PR! By analyzing the history of the files in this pull request, we identified @butonic, @nickvergessen and @MorrisJobke to be potential reviewers.

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Mar 28, 2017
@icewind1991 icewind1991 added this to the Nextcloud 12.0 milestone Mar 28, 2017
@icewind1991
Copy link
Member Author

22eeff6 is relevant part, the rest is #4112

@icewind1991 icewind1991 changed the title Cache switch authentication token in memcache Cache swift authentication token in memcache Mar 28, 2017
LukasReschke
LukasReschke reviewed Mar 28, 2017
View reviewed changes
lib/private/Files/ObjectStore/Swift.php Outdated
$serviceName = null;
$cachedTokenString = $this->memcache->get('token');
if ($cachedTokenString) {
$cachedToken = unserialize($cachedTokenString);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/nextcloud/3rdparty/blob/f5555fef8e80d8380efb44dc8b7622a1de573c15/rackspace/php-opencloud/lib/OpenCloud/OpenStack.php#L398-L409 claims this is an array. Can we use json_encode and json_decode thus? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the array contains objects, so we can't json it

@icewind1991 icewind1991 force-pushed the swift-cache-token branch 3 times, most recently from 75c9a30 to 67fe4b2 Compare March 28, 2017 15:15
@MorrisJobke
Copy link
Member

@icewind1991 Conflicts :/

@icewind1991 icewind1991 force-pushed the swift-cache-token branch 3 times, most recently from ad7a7b5 to e3fe734 Compare March 28, 2017 16:02
@icewind1991
Copy link
Member Author

rebased

@codecov-io
Copy link

codecov-io commented Mar 28, 2017 •
edited
Loading

Codecov Report

Merging #4116 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #4116      +/-   ##
============================================
- Coverage      54.2%   54.18%   -0.03%     
- Complexity    21294    21300       +6     
============================================
  Files          1310     1310              
  Lines         81253    81285      +32     
  Branches       1285     1285              
============================================
+ Hits          44043    44044       +1     
- Misses        37210    37241      +31
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/ObjectStore/Swift.php 0% <0%> (ø) 39 <4> (+6) ⬆️
apps/files_trashbin/lib/Trashbin.php 73.39% <0%> (-0.25%) 133% <0%> (ø)
apps/comments/lib/EventHandler.php 87.5% <0%> (+8.33%) 7% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e26f138...6991b79. Read the comment docs.

MorrisJobke
MorrisJobke approved these changes Mar 28, 2017
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works 👍

@MorrisJobke
Copy link
Member

Updated the merged 3rdparty submodule.

@icewind1991
Copy link
Member Author

@LukasReschke switched to json

@MorrisJobke please retest

MorrisJobke
MorrisJobke approved these changes Mar 28, 2017
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still works 👍

@rullzer rullzer merged commit 626d03e into master Mar 29, 2017
@rullzer rullzer deleted the swift-cache-token branch March 29, 2017 09:23
@icewind1991 icewind1991 mentioned this pull request Apr 21, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukasReschke LukasReschke LukasReschke left review comments

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@rullzer rullzer rullzer approved these changes

Assignees
No one assigned
Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 12.0
Development

Successfully merging this pull request may close these issues.
6 participants
@icewind1991 @mention-bot @MorrisJobke @codecov-io @rullzer @LukasReschke